[codex] resolve environment shell metadata eagerly - #27709
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f285bd8305
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| shell: environment | ||
| .shell | ||
| .clone() | ||
| .as_ref() | ||
| .map(|shell| shell.name().to_string()) | ||
| .unwrap_or_else(|| shell.name().to_string()), |
There was a problem hiding this comment.
When the remote environment shell differs from the host shell (for example the local session is zsh but the exec-server reports bash), this now advertises the remote shell in <environment_context>, but exec_command still builds commands from session.user_shell() before sending them to the selected environment. The model will omit an explicit shell because it was told bash is available, yet the runtime can still invoke the host shell path on the remote environment and fail before the command runs.
Useful? React with 👍 / 👎.
| .unwrap_or_else(|err| { | ||
| warn!("failed to resolve turn environments: {err}"); | ||
| ResolvedTurnEnvironments::default() |
There was a problem hiding this comment.
For callers that reach Op::UserInput without the app-server pre-validation, an unknown or duplicate thread_settings.environments value is now applied to session_configuration and then silently converted to an empty environment list here. That turns a bad turn override into a successful local/default turn and leaves the session with the invalid environment selection, whereas this path used to return InvalidRequest before mutating state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ok for the sake of simplification
| .ok_or_else(|| { | ||
| CodexErr::InvalidRequest(format!("unknown turn environment id `{environment_id}`")) | ||
| })?; | ||
| let shell = match environment.info().await { |
There was a problem hiding this comment.
When a selected remote exec-server is offline or slow to initialize, this environment.info().await forces the lazy remote client to connect just to populate an optional shell name. Because callers use this during thread/turn setup and the error is only swallowed after the connect/initialize attempt, a configured remote environment can add the full transport timeout to startup or every turn before the user even invokes a remote tool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ok for now. We'll fix this more holistically later
Why
Turn construction passed resolved environments through several layers while leaving the environment shell unresolved. As a result, model-visible environment context could fall back to the session shell instead of reporting the selected remote environment's shell.
Resolve environment metadata at the turn-context boundary so each turn carries the shell that belongs to its selected environment. Keep request validation in app-server, where invalid selections can be returned as straightforward JSON-RPC errors without coupling core turn construction to that policy.
What changed
new_turn_context_from_configurationShellon eachTurnEnvironmenttest_codexturn and verifies the model-visible environment message reportsbashTesting
cargo check -p codex-core --test all -p codex-app-serverremote_test_env_exposes_bash_shell_to_modelon the Linux remote-executor harness